Add MOABB Dataset Integration - #74

brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py (1)

56-84: Consider using Optional[str] for type annotation consistency.

The subject_id parameter defaults to None but is typed as str, which violates PEP 484. This should be Optional[str] for type safety.

Also note: this implementation uses setattr to attach subject assignments (lines 81-82), while the schalk_wolpaw_physionet_2009 pipeline uses **subject_assignments in the Data constructor. Consider aligning the approaches for consistency across pipelines.

♻️ Suggested type annotation fix

+from typing import Optional
+
 ...
 
-    def _generate_splits(self, trials, subject_id: str = None):
+    def _generate_splits(self, trials, subject_id: Optional[str] = None):

brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py (1)

68-103: Consider using Optional[str] for type annotation.

Same as the BI2014a pipeline - the subject_id parameter should be annotated as Optional[str] for PEP 484 compliance.

The implementation correctly generates task-specific stratified splits for all three task configurations and adds subject-level assignments when applicable.

♻️ Suggested type annotation fix

+from typing import Optional
+
 ...
 
-    def _generate_splits(self, trials, subject_id: str = None):
+    def _generate_splits(self, trials, subject_id: Optional[str] = None):

brainsets/moabb_pipeline.py (3)

150-205: Double data fetch is inefficient.

The paradigm.get_data() is called twice - once with return_epochs=False (line 153) and again with return_epochs=True (line 191). This downloads/processes the data twice, which could be slow for large datasets.

Consider fetching with return_epochs=True once and extracting the array data from epochs, or caching the result.

♻️ Suggested optimization

-        X, labels, meta = paradigm.get_data(
-            dataset=dataset,
-            subjects=[subject],
-            return_epochs=False,
-        )
-
-        # ... validation and filtering ...
-
-        epochs, _, meta_epochs = paradigm.get_data(
+        # Fetch epochs once and extract arrays
+        epochs_list, labels, meta = paradigm.get_data(
             dataset=dataset,
             subjects=[subject],
             return_epochs=True,
         )
+
+        # Extract array data from epochs
+        # X can be derived from epochs_list after session filtering

Note: The exact refactor depends on how MOABB structures the epoch data. You may need to concatenate epoch arrays after filtering.

---

207-235: Consider documenting or using MNE constants for channel type mapping.

The magic numbers in ch_type_map (lines 225-231) correspond to MNE channel kinds, but this isn't immediately obvious. Consider adding a comment referencing MNE's channel kind constants or importing them directly.

---

386-410: Unused subject_id parameter is an intentional extension point.

The subject_id parameter is unused in the base implementation but provides an extension point for subclasses to add subject-level splits. This is a valid pattern, though it would benefit from a brief comment explaining this design choice.

The type annotation should use Optional[str] per PEP 484.

♻️ Suggested improvement

-    def _generate_splits(self, trials, subject_id: str = None):
+    def _generate_splits(self, trials, subject_id: Optional[str] = None):
         """Generate stratified folds for trials.
 
         Parameters
         ----------
         trials : Interval
             Trial intervals with label fields
         subject_id : str, optional
-            Subject identifier for subject-level splits. Default is None.
+            Subject identifier for subject-level splits. Unused in base class
+            but available for subclass extensions. Default is None.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

brainsets/utils/moabb/pipeline.py (3)

96-97: Consider annotating mutable class attributes with ClassVar.

Mutable class attributes like {} can lead to unintended state sharing if mutated in-place rather than reassigned. While subclasses typically override these entirely, adding ClassVar clarifies intent and prevents accidental mutation.

♻️ Suggested improvement

+from typing import ClassVar
 ...
-    dataset_kwargs: Dict[str, Any] = {}
-    paradigm_kwargs: Dict[str, Any] = {}
+    dataset_kwargs: ClassVar[Dict[str, Any]] = {}
+    paradigm_kwargs: ClassVar[Dict[str, Any]] = {}

---

319-343: Consider using MNE's built-in channel type detection.

The hard-coded FIFF constants are fragile and may not cover all channel types. MNE provides mne.channel_type(info, idx) which handles this comprehensively.

♻️ Suggested refactor using MNE API

     def _get_channel_types(self, info):
-        # MNE channel kind constants (mne.io.constants.FIFF)
-        ch_type_map = {
-            2: "EEG",  # FIFFV_EEG_CH
-            202: "EOG",  # FIFFV_EOG_CH
-            302: "EMG",  # FIFFV_EMG_CH
-            402: "ECG",  # FIFFV_ECG_CH
-            502: "MISC",  # FIFFV_MISC_CH
-        }
-        return [
-            ch_type_map.get(info["chs"][info["ch_names"].index(ch)]["kind"], "MISC")
-            for ch in info["ch_names"]
-        ]
+        return [mne.channel_type(info, idx).upper() for idx in range(len(info["ch_names"]))]

---

383-391: Unknown labels silently mapped to -1.

Labels not found in self.label_map are silently assigned ID -1 (line 391). This could mask configuration errors or unexpected events in the data.

Consider logging a warning when unknown labels are encountered.

🔧 Suggested improvement

-        id_values = np.array([self.label_map.get(label, -1) for label in labels])
+        id_values = []
+        for label in labels:
+            id_val = self.label_map.get(label, -1)
+            if id_val == -1:
+                logging.warning(f"Unknown label '{label}' mapped to ID -1")
+            id_values.append(id_val)
+        id_values = np.array(id_values)

brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py (1)

76-76: subject_id: str = None should be Optional[str] = None.

Same implicit Optional (RUF013) as in the korczowski pipeline.

♻️ Proposed fix

-    def generate_splits(self, trials, subject_id: str = None):
+    def generate_splits(self, trials, subject_id: Optional[str] = None):

Also add Optional to the imports at the top of the file:

+from typing import Optional

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py` at line 76, The
function signature generate_splits(self, trials, subject_id: str = None) should
use Optional for clarity and type-checking; change it to subject_id:
Optional[str] = None and add Optional to the typing imports at the top of the
file (ensure Optional is imported alongside any existing typing imports). Update
the function definition for generate_splits and the import statement to
reference Optional so static checkers no longer flag the implicit optional type.

brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py (1)

63-90: Use Optional[str] instead of implicit str = None, and prefer passing subject assignments through the Data constructor.

Two nits:

1. subject_id: str = None is PEP 484-invalid implicit Optional (Ruff RUF013).
2. The physionet pipeline composes the Data object once via Data(**task_splits, **subject_assignments, domain=trials). This pipeline instead calls super() then mutates the result with setattr, which is inconsistent and fragile if temporaldata.Data ever adds __slots__ or a frozen mode.

♻️ Proposed fix

-    def generate_splits(self, trials, subject_id: str = None):
+    def generate_splits(self, trials, subject_id: Optional[str] = None):
         ...
         splits = super().generate_splits(trials, subject_id=subject_id)
 
         if subject_id is not None:
             subject_assignments = generate_subject_kfold_assignment(
                 subject_id, n_folds=3, val_ratio=0.2, seed=42
             )
-            for key, value in subject_assignments.items():
-                setattr(splits, key, value)
+            # Re-build to include subject assignments in a single constructor call
+            folds_dict = {f"fold_{i}": getattr(splits, f"fold_{i}") for i in range(3)}
+            splits = Data(**folds_dict, **subject_assignments, domain=trials)
 
         return splits

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py` around lines
63 - 90, The generate_splits method uses an implicit Optional by declaring
subject_id: str = None and mutates the returned splits with setattr which is
fragile; change the signature to subject_id: Optional[str] and, after calling
super().generate_splits(...), merge subject_assignments into the Data object by
constructing a new Data from combined dicts instead of mutating splits (i.e.,
call generate_subject_kfold_assignment(...) when subject_id is not None, then
create/return Data(**splits.__dict__ or splits.as_dict(), **subject_assignments,
domain=trials) or the equivalent Data constructor used elsewhere to keep
composition consistent and avoid setattr/frozen/slots issues).

brainsets/utils/moabb/pipeline.py (2)

90-91: Mutable dict class attributes should be annotated with ClassVar or use field(default_factory=...).

dataset_kwargs and paradigm_kwargs are shared mutable dicts at the class level. If any code path ever mutates them (e.g., cls.dataset_kwargs["key"] = val), the mutation leaks across all subclasses that don't override them.

♻️ Proposed fix

+from typing import ClassVar
 ...
-    dataset_kwargs: Dict[str, Any] = {}
-    paradigm_kwargs: Dict[str, Any] = {}
+    dataset_kwargs: ClassVar[Dict[str, Any]] = {}
+    paradigm_kwargs: ClassVar[Dict[str, Any]] = {}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@brainsets/utils/moabb/pipeline.py` around lines 90 - 91, dataset_kwargs and
paradigm_kwargs are currently mutable dicts defined at class level which can
leak mutations across subclasses; either mark them as class-level constants by
annotating them as ClassVar[Dict[str, Any]] (e.g., dataset_kwargs:
ClassVar[Dict[str, Any]] = {}) if you intend them to be shared and not mutated
per-instance, or make them per-instance dataclass fields using
field(default_factory=dict) (e.g., dataset_kwargs: Dict[str, Any] =
field(default_factory=dict)) so each instance gets its own dict; update the
declarations for dataset_kwargs and paradigm_kwargs accordingly in pipeline.py
and adjust any code that mutates them to use instance attributes if you choose
the default_factory approach.

---

355-356: Replace the private mne.utils._get_stim_channel with the public mne.pick_types API to avoid tight coupling to MNE internals.

The private function exists in MNE 1.11.0, but using it couples the code to internal implementation details. Since the code only checks whether stim channels exist (not their names), the public API achieves the same result.

♻️ Suggested refactor

-        stim_channels = mne.utils._get_stim_channel(None, raw.info, raise_error=False)
-        if len(stim_channels) > 0:
+        stim_indices = mne.pick_types(raw.info, meg=False, eeg=False, stim=True)
+        if len(stim_indices) > 0:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@brainsets/utils/moabb/pipeline.py` around lines 355 - 356, Replace the
private call to mne.utils._get_stim_channel by using the public picker: call
mne.pick_types(raw.info, stim=True) (e.g., assign to stim_picks or reuse
stim_channels name) and then check if len(stim_picks) > 0; this removes the
dependency on the internal mne.utils._get_stim_channel while preserving the
logic that detects presence of stim channels.